Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MergeV, MergeE, & Option Steps #214

Open
wants to merge 52 commits into
base: master
Choose a base branch
from
Open

MergeV, MergeE, & Option Steps #214

wants to merge 52 commits into from

Conversation

criminosis
Copy link
Contributor

@criminosis criminosis commented Jul 13, 2024 •
edited
Loading

This PR does the following:

  • Changed property() step to key by Into, this allows custom vertex ids to be assigned
    • Also updated property_many, property_with_cardinality, and property_many_with_cardinality
  • Added new steps: mergeV, mergeE, option
    • Added support for using option for mergeV & mergeE steps, see integration_traversal.rs for examples
    • Added support for using the to & from options for mergeE as well
  • Implemented T::id into a GKey to support injected map parameters for mergeV & mergeE
    • Also facilitates addV("some_label").properties(T::id, "string_or_integer_id_here")...
  • Added JG to docker compose environment for custom string vertex id tests
    • @wolf4ood the JG container doesn't participate in gremlin version matrix, so the custom vertex ids tests are being ran in each matrix's entry (and JG is ignoring the specified Tinkerpop version). Happy to make changes for how you'd think it'd be better to do. Thought about adding another feature flag for those tests, but figured it'd be good to get your thoughts first.
    • Added a feature to selectively run merge tests for TP versions != 3.5.6
      • Assumes != 3.5.6 will be a higher version. The merge steps don't exist in that family of Tinkerpop.
  • Added mergeV & mergeE to WRITE_OPERATORS in bytecode.rs
    • @wolf4ood this seems like an investigative call upon the bytecode so just stuck them in, lemme know if there's more to do.
  • Support choose step options with literals
  • Support g:Column via by() step
  • Reworked sideEffect() step implementation
  • Permit properties() step in an anonymous traversal
  • Modified Websocket connection layer error handling and propagation
  • Updated to mobc 0.8 family
  • Implemented None step
  • Implemented Tinkerpop-ism of iterator() on returned stream for async client

@criminosis
First pass
e2c85cf
@criminosis
Added JanusGraph in memory mode to docker compose enviornment
8486503
@criminosis
Added mergeV as a start step
e06fe55
@criminosis
Relaxed property step keys to Into<GValue> from &str
b569530 
support converting a TraversalBuilder into GValue via Bytecode
@criminosis
Added JanusGraph custom vertex id tests
5ec7c71
@criminosis
Implemented From<HashMap<GKey, GValue>> for MergeVertexStep to suppor…
7cf15cd 
…t literal maps being defined for mergeV steps
@criminosis
Implemented Option step for mergeV
6a0f4c5
@criminosis
Added healthcheck for JG & wait with timeout for docker compose up in…
0f4aee6 
… GH Action
@criminosis
Use Docker Compose v2 via "docker compose" vs v1's "docker-compose" i…
dddf8c0 
…n order to leverage v2's wait flag
@criminosis
Combine merge v custom id test cases
e1d7096
@criminosis
Better handle tests being reran
3d3a300
@criminosis
Added merge_v_tests feature for tests
6f56392
@criminosis
Formatting
dada863
@criminosis
FIxed if condition
bc94862
@criminosis
Corrected cargo.toml merge_test feature
80357fe
@criminosis
Changed GH Action if statement formatting
18051d3
@criminosis
Increased docker compose timeout time for healthy service check
6737d91
@criminosis
Fixed imports for merge_test module. Moved merge tests into their own…
7a8a0cd 
… module
@criminosis
Use drop vertices test utility function
097fa04
@criminosis
Drop vertices for test_merge_v_options
aff1d27
@criminosis
Added mergeV step to anonymous traversals
44cdfd3
@criminosis
Implemented mergeE step
1c97441
@criminosis
Added mergeV and mergeE to Bytecode WRITE_OPERATORS
8b28edc
@criminosis
Implemented travsal test based on reference doc combo mergeV and mergeE
72615b9
@criminosis
Support literal options for choose step and added test
edffffb
@criminosis
Copy link
Contributor Author

@wolf4ood didn't realize this got over 1k lines 😅 , let me know what you think. Happy to make changes 👍 .

This was referenced Jul 13, 2024
Open
Open
@criminosis criminosis changed the title New steps MergeV, MergeE, Option Steps Jul 13, 2024
@criminosis criminosis changed the title MergeV, MergeE, Option Steps MergeV, MergeE, & Option Steps Jul 13, 2024
@criminosis
Copy link
Contributor Author

criminosis commented Aug 2, 2024 •
edited
Loading

@wolf4ood I think I've put in some missing logic in commit f8f5fec if the websocket peer closes the async connection. Basically mirrored sync version over here:

gremlin-rs/gremlin-client/src/connection.rs

Lines 289 to 306 in d7ce0e4

pub fn send(&mut self, payload: Vec<u8>) -> GremlinResult<()> {
self.stream.send(payload).map_err(|e| {
if let GremlinError::WebSocket(_) = e {
self.broken = true;
}
e
})
}
pub fn recv(&mut self) -> GremlinResult<Vec<u8>> {
self.stream.recv().map_err(|e| {
if let GremlinError::WebSocket(_) = e {
self.broken = true
}
e
})
}

I've been having issues where a peer would reset after a long duration of writes but seemed like I kept getting the same broken connection for my retries. Figured I'd tuck it into this PR since it was still open.

@criminosis
Map additional tungstenite errors to GremlinError::WebSocketAsync
668c564
@criminosis
Arc tungstenite::Error into WebSocketAsync error to maintain Async er…
cdcc212 
…ror enum back to caller
@criminosis
Expose healthcheck interval setting on async connection pool
2b72a1a
@criminosis
Formatting
093c04c
@criminosis
Map mobc pool errors to type that would invalidate connection
802a20c
@criminosis
Exploratory logging
03c24fe
@criminosis
Added uuid to connection instance logging
a342d8c
@criminosis
Update mobc and make its idle connection behavior the same as the rd2…
ec1437d 
…d sync pool
@criminosis
0.8 mobc does not treat async-std feature as mutually exclusive from …
784ca87 
…tokio
@criminosis
Include tokio/sync for mobc compilation in async-std-runtime feature
c10ceaa
@criminosis
Implemented None step
6a94a88
@criminosis
Implemented iterator() method on returned remote stream to consume st…
bce3737 
…ream for only Null terminated traversals
@criminosis
Trial connection multiplexing for non-credential configured clients
5db2b58
@criminosis
Formatting
8082d3b
@criminosis
Removed internal channel bounding
7500820
@wolf4ood
Copy link
Owner

Hey @criminosis

thanks for the hard work let me know when the PR is ready for review i will take a look :)

@criminosis
Copy link
Contributor Author

Yeah I started tinkering with other stuff 😅 .

Lemme back out the exploratory logging & and the experimental connection muxing.

Are you okay with the rest of the stuff going in as a single PR? Might take a while to splice everything else out at this point 🤔

@wolf4ood
Copy link
Owner

@criminosis

if it's ready for review as it is i would keep it in a single PR, I might have some time in the next days to go through it

criminosis
criminosis commented Sep 27, 2024
View reviewed changes
.github/workflows/coverage.yml
@@ -23,7 +23,7 @@ jobs:
- uses: actions/checkout@v2
- name: Starting Gremlin Servers
run: |
docker-compose -f ./docker-compose/docker-compose.yaml up -d
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems docker-compose (note the middle - is no longer a known command on the latest action runners, so changed it here and in the test workflow files

@criminosis
Copy link
Contributor Author

@wolf4ood think I've got it tuned back to being in a ready to merge state 👍 . I updated the OP with I think the additional items since I stopped updating it.

Whenever you're able to review it and it gets merged mind cutting a release please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@criminosis @wolf4ood